sql, server: add skeleton TokenBucket connector and tenant resource limits configuration APIs #67768

RaduBerinde · 2021-07-19T20:15:17Z

This PR is a scaled back version of #67508 where we don't use the system table at all. It's meant to put some of the infrastructure pieces in place and provide a stub API for reconfiguration.

The plan is to add consumption metrics on top of this soon so that CC can develop in parallel.

server: add TokenBucket connector API

This change adds the TokenBucket API proposed in the RFC (#66436), a
stub implementation and client for it, and the corresponding KV
connector interface.

The client and server-side code lives in
ccl/multitenantccl/tenantcostclient and tenantcostserver.

Release note: None

sql: tenant resource limits configuration API

This commit adds a crdb_internal.update_tenant_resource_limits
internal SQL function (to be used by the system tenant) which updates
the token bucket configuration for a specific tenant.

Release note: None

cockroach-teamcity · 2021-07-19T20:15:25Z

This change is

RaduBerinde · 2021-07-19T21:15:33Z

@kernfeld-cockroach please take a look at the configuration function:
https://github.com/cockroachdb/cockroach/pull/67768/files#diff-7c2fd288d1311559143c751945897a99a885f3f20df93d95fa1dee3c4b5c6049R5088

I have dropped the operation UUID from there. We have an alternate proposal for the internal state and we won't support that operation UUID anymore.

kernfeld-cockroach · 2021-07-20T18:18:06Z

Thanks for the heads-up. I am good with the current proposed function signature.

Since we're planning to call this function every 10 seconds, I don't think it matters that much to be able to reject out-of-date or duplicate requests. But if we wanted to leave forwards compatibility for that functionality, I would suggest that we include a monotonically increasing 64-bit integer nonce value. CRDB would ignore it for the MVP. But also, if there's not going to be a reasonable place to store the value anyways then there's likely no point adding it.

ajwerner

But if we wanted to leave forwards compatibility for that functionality, I would suggest that we include a monotonically increasing 64-bit integer nonce value. CRDB would ignore it for the MVP. But also, if there's not going to be a reasonable place to store the value anyways then there's likely no point adding it.

I assumed that the implementation of the client library would be tracking such a nonce inside the implementation of TenantSideCostController. Such a thing will need to exist. The SQL pod shouldn't be calling the gRPC method directly IIUC.

Reviewed 32 of 32 files at r1, 12 of 12 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)

pkg/ccl/kvccl/kvtenantccl/connector.go, line 396 at r1 (raw file):

Quoted 5 lines of code…

	for ctx.Err() == nil {
		client, err := c.getClient(ctx)
		if err != nil {
			continue
		}

No logging or backoff?

pkg/ccl/kvccl/kvtenantccl/connector.go, line 399 at r1 (raw file):

		resp, err := client.TokenBucket(ctx, in)
		if err != nil {
			log.Warningf(ctx, "error issuing RangeLookup RPC: %v", err)

I see now that this is copy-pasta. Should we refactor out a helper?

pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go, line 41 at r1 (raw file):

	if err := stopper.RunAsyncTask(context.Background(), "refresher", func(ctx context.Context) {
		c.mainLoop(ctx)
	}); err != nil {

Thoughts on a start method? I generally dislike goroutines running under a stopper before calls to Start. I know they exist but I don't like them.

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 55 at r1 (raw file):

Quoted 4 lines of code…

		if err := updateTenantUsageState(ctx, s.executor, txn, tenantID, state); err != nil {
			return err
		}
		return nil

nit: return updateTenantUsageState(ctx, s.executor, txn, tenantID, state)

pkg/multitenant/tenant_usage.go, line 11 at r2 (raw file):

// licenses/APL.txt.

package multitenant

nit: add a doc.go or a package comment here

pkg/server/tenant.go, line 392 at r1 (raw file):

Quoted 5 lines of code…

// TenantSideCostController is an interface through which tenants report and
// throttle resource usage.
type TenantSideCostController interface {
	// TODO(radu)
}

Why define this here and not in the multitenant package I was going to suggest you create but you already did in the second commit?

andy-kimball

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 34 at r2 (raw file):

	result := &roachpb.TokenBucketResponse{}
	if err := s.db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {

How come it isn't possible to make this a single SQL statement that selects the current state and then updates it?

pkg/ccl/multitenantccl/tenantcostserver/tenanttokenbucket/tenant_token_bucket.go, line 23 at r2 (raw file):

type State struct {
	// Burst limit in RUs.
	RUBurstLimit float64

super nit: idiomatic Go would use field names as the first word in each comment.

RaduBerinde

TFTRs!

I assumed that the implementation of the client library would be tracking such a nonce inside the implementation of TenantSideCostController

Correct, the above is referring to the host-side reconfiguration API (for changing the refill rate etc)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @andy-kimball)

pkg/ccl/kvccl/kvtenantccl/connector.go, line 399 at r1 (raw file):

Previously, ajwerner wrote…

I see now that this is copy-pasta. Should we refactor out a helper?

I tried but couldn't find a clean way to do it; other functions have some "hard error" return paths that are hard to incorporate in a func. I agree it should be done but it will require some back and forth so I'll leave that out of this PR.

Fixed the RPC name.

pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go, line 41 at r1 (raw file):

Previously, ajwerner wrote…

	if err := stopper.RunAsyncTask(context.Background(), "refresher", func(ctx context.Context) {
		c.mainLoop(ctx)
	}); err != nil {
Thoughts on a start method? I generally dislike goroutines running under a stopper before calls to Start. I know they exist but I don't like them.

Good point, done.

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 34 at r2 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

How come it isn't possible to make this a single SQL statement that selects the current state and then updates it?

How? By having some internal SQL function?

I don't know how we could make that work with the new proposal, where we have to interact with two rows.

In any case, I will start with a simple implementation and take it from there.

pkg/ccl/multitenantccl/tenantcostserver/tenanttokenbucket/tenant_token_bucket.go, line 23 at r2 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

super nit: idiomatic Go would use field names as the first word in each comment.

Done.

pkg/server/tenant.go, line 392 at r1 (raw file):

Previously, ajwerner wrote…

// TenantSideCostController is an interface through which tenants report and
// throttle resource usage.
type TenantSideCostController interface {
	// TODO(radu)
}
Why define this here and not in the multitenant package I was going to suggest you create but you already did in the second commit?

Done.

andy-kimball

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @andy-kimball, and @RaduBerinde)

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 34 at r2 (raw file):

Previously, RaduBerinde wrote…

How? By having some internal SQL function?

I don't know how we could make that work with the new proposal, where we have to interact with two rows.

In any case, I will start with a simple implementation and take it from there.

I just mean an INSERT INTO foo SELECT ... FROM foo kind of thing. Doesn't the code you have here run into serializability conflicts when multiple SQL pods call at the same time? If it were a single INSERT/SELECT statement that used FOR UPDATE locking, we'd never hit conflicts (I suppose we could use FOR UPDATE in readTenantUsage as well). But maybe I'm not understanding exactly what readTenantUsageState and updateTenantUsageState will be doing - my assumption is that we read 1 row, then use the values in that row to create another (new) row.

RaduBerinde

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @andy-kimball)

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 34 at r2 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

I just mean an INSERT INTO foo SELECT ... FROM foo kind of thing. Doesn't the code you have here run into serializability conflicts when multiple SQL pods call at the same time? If it were a single INSERT/SELECT statement that used FOR UPDATE locking, we'd never hit conflicts (I suppose we could use FOR UPDATE in readTenantUsage as well). But maybe I'm not understanding exactly what readTenantUsageState and updateTenantUsageState will be doing - my assumption is that we read 1 row, then use the values in that row to create another (new) row.

I will write up the new approach in the RFC when I get a chance (Andrew has a nice writeup in there, though we improved it a bit offline). We will be reading and updating two rows (but with no secondary index). We will use SELECT FOR UPDATE.

andy-kimball

I'm not too familiar with the surrounding code, but it with some small comments/questions. I'm approving now because I don't want you to get blocked too much longer, but hope that someone else in SQL can approve soon as well.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @andy-kimball, and @RaduBerinde)

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 34 at r2 (raw file):

Previously, RaduBerinde wrote…

I will write up the new approach in the RFC when I get a chance (Andrew has a nice writeup in there, though we improved it a bit offline). We will be reading and updating two rows (but with no secondary index). We will use SELECT FOR UPDATE.

OK, I just want to confirm we're understanding each other: I'm suggesting that all this code could just be a single SQL statement that round-trips just once to the DB rather than twice, thus reducing how long we hold locks:

INSERT INTO the_tenant_usage_table
SELECT u.Seq+1, u.RU+$1, u.ReadRequests+$2, u.ReadBytes+$3, ...
FROM the_tenant_usage_table u
WHERE its_the_existing_last_row()

And you're saying that this approach won't work for some reason, or maybe that it's too complex, right? It's fine if that's the case, I just want to make sure we're not mis-communicating.

pkg/server/server.go, line 639 at r6 (raw file):

		tenantUsage,
	)
	node.admissionQ = gcoord.GetWorkQueue(admission.KVWork)

Looks like admissionQ is getting set twice here, once in the constructor, once after.

Also, is it possible to not need to create a TenantUsageServer if this is not a multi-tenant cluster? Or will it be cheap enough that it doesn't matter?

ajwerner

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @andy-kimball, and @RaduBerinde)

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 34 at r2 (raw file):

Quoted 4 lines of code…

INSERT INTO the_tenant_usage_table
SELECT u.Seq+1, u.RU+$1, u.ReadRequests+$2, u.ReadBytes+$3, ...
FROM the_tenant_usage_table u
WHERE its_the_existing_last_row()

Whether you do it in a single statement and write it with the logic in SQL or do it in two statements, one which reads and the other which writes, it's roughly the same number of round-trips to the store. The advantage of the single-statement formulation is that I think we'll tell KV to commit in batch and thus hit the pure 1PC path with no intents.

andy-kimball · 2021-07-21T23:03:15Z

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 34 at r2 (raw file):

Previously, ajwerner wrote…

INSERT INTO the_tenant_usage_table
SELECT u.Seq+1, u.RU+$1, u.ReadRequests+$2, u.ReadBytes+$3, ...
FROM the_tenant_usage_table u
WHERE its_the_existing_last_row()
Whether you do it in a single statement and write it with the logic in SQL or do it in two statements, one which reads and the other which writes, it's roughly the same number of round-trips to the store. The advantage of the single-statement formulation is that I think we'll tell KV to commit in batch and thus hit the pure 1PC path with no intents.

Yes, I was thinking of round-trips from an app tier to SQL, which doesn't apply here, since this would all be running in a built-in function on a SQL node. However, I think there's actually 3 round-trips instead of 2, since committing the explicit transaction requires one extra round-trip.

This change adds the TokenBucket API proposed in the RFC (cockroachdb#66436), a stub implementation and client for it, and the corresponding KV connector interface. The client and server-side code lives in ccl/multitenantccl/tenantcostclient and tenantcostserver. Release note: None

This commit adds a `crdb_internal.update_tenant_resource_limits` internal SQL function (to be used by the system tenant) which updates the token bucket configuration for a specific tenant. Release note: None

RaduBerinde

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @andy-kimball)

pkg/ccl/multitenantccl/tenantcostserver/token_bucket.go, line 34 at r2 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Yes, I was thinking of round-trips from an app tier to SQL, which doesn't apply here, since this would all be running in a built-in function on a SQL node. However, I think there's actually 3 round-trips instead of 2, since committing the explicit transaction requires one extra round-trip.

Yeah I understand, I'll keep it in mind and see if we can do something like that once we're further in the implementation.

It would be nice if we had a way to set up the transaction to autocommit on the UPDATE statement.

pkg/server/server.go, line 639 at r6 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Looks like admissionQ is getting set twice here, once in the constructor, once after.

Also, is it possible to not need to create a TenantUsageServer if this is not a multi-tenant cluster? Or will it be cheap enough that it doesn't matter?

Fixed the admissionQ.

Is there an easy way to tell if this is a multi-tenant cluster?

I don't think this server will do any background work if there are no tenant requests coming through, so it shouldn't be a problem.

RaduBerinde · 2021-07-22T15:52:24Z

bors r+

craig · 2021-07-22T17:08:51Z

This PR was included in a batch that was canceled, it will be automatically retried

RaduBerinde · 2021-07-22T18:31:13Z

bors r-

craig · 2021-07-22T18:31:15Z

Canceled.

RaduBerinde · 2021-07-22T18:31:26Z

bors r+

craig · 2021-07-22T19:52:29Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2021-07-22T22:10:53Z

Build succeeded:

GitHub CI (Cockroach)

67873: multitenant: expose consumption metrics r=RaduBerinde a=RaduBerinde This PR is for the top two commits. The rest is #67768. #### util: add AggGaugeFloat64 This commit adds a float aggregate gauge. We also add support for incrementing a GaugeFloat64 (required for the aggregate gauge). Release note: None #### multitenant: expose consumption metrics This commit adds per-tenant consumption metrics. Each node presents the latest consumption value that it knows about, for each tenant. The higher-level logic will take the Max value across all nodes. The metrics are: - tenant.consumption.request_units - tenant.consumption.read_requests - tenant.consumption.read_bytes - tenant.consumption.write_requests - tenant.consumption.write_bytes - tenant.consumption.sql_pods_cpu_seconds Note that these are aggregate metrics, meaning that we export a separate value for each tenant, and also a sum across all tenants. The sums are not meaningful in this case and should not be used. Release note: None Co-authored-by: Radu Berinde <[email protected]>

RaduBerinde requested a review from ajwerner July 19, 2021 20:15

RaduBerinde requested a review from a team as a code owner July 19, 2021 20:15

RaduBerinde force-pushed the partial-tmp branch from d18bbbe to 9fec1c8 Compare July 19, 2021 21:13

RaduBerinde force-pushed the partial-tmp branch 2 times, most recently from 7a55434 to 9860a0c Compare July 20, 2021 17:49

RaduBerinde force-pushed the partial-tmp branch from 9860a0c to 489c40b Compare July 20, 2021 20:28

ajwerner reviewed Jul 20, 2021

View reviewed changes

andy-kimball reviewed Jul 20, 2021

View reviewed changes

RaduBerinde force-pushed the partial-tmp branch from 489c40b to d6ca6b4 Compare July 20, 2021 22:01

RaduBerinde commented Jul 20, 2021

View reviewed changes

andy-kimball reviewed Jul 20, 2021

View reviewed changes

RaduBerinde commented Jul 21, 2021

View reviewed changes

RaduBerinde force-pushed the partial-tmp branch from d6ca6b4 to 8cc783c Compare July 21, 2021 19:40

RaduBerinde mentioned this pull request Jul 21, 2021

multitenant: expose consumption metrics #67873

Merged

andy-kimball approved these changes Jul 21, 2021

View reviewed changes

ajwerner reviewed Jul 21, 2021

View reviewed changes

RaduBerinde added 2 commits July 21, 2021 21:43

sql: tenant resource limits configuration API

1a47f6b

This commit adds a `crdb_internal.update_tenant_resource_limits` internal SQL function (to be used by the system tenant) which updates the token bucket configuration for a specific tenant. Release note: None

RaduBerinde commented Jul 22, 2021

View reviewed changes

RaduBerinde force-pushed the partial-tmp branch from 8cc783c to 1a47f6b Compare July 22, 2021 04:47

craig bot merged commit 0119b2b into cockroachdb:master Jul 22, 2021

tbg mentioned this pull request Jul 26, 2021

roachtest: multitenant-upgrade failed #65830

Closed

RaduBerinde deleted the partial-tmp branch July 28, 2021 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql, server: add skeleton TokenBucket connector and tenant resource limits configuration APIs #67768

sql, server: add skeleton TokenBucket connector and tenant resource limits configuration APIs #67768

RaduBerinde commented Jul 19, 2021

cockroach-teamcity commented Jul 19, 2021

RaduBerinde commented Jul 19, 2021

kernfeld-cockroach commented Jul 20, 2021

ajwerner left a comment

andy-kimball left a comment

RaduBerinde left a comment

andy-kimball left a comment

RaduBerinde left a comment

andy-kimball left a comment

ajwerner left a comment

andy-kimball commented Jul 21, 2021

RaduBerinde left a comment

RaduBerinde commented Jul 22, 2021

craig bot commented Jul 22, 2021

RaduBerinde commented Jul 22, 2021

craig bot commented Jul 22, 2021

RaduBerinde commented Jul 22, 2021

craig bot commented Jul 22, 2021

craig bot commented Jul 22, 2021

sql, server: add skeleton TokenBucket connector and tenant resource limits configuration APIs #67768

sql, server: add skeleton TokenBucket connector and tenant resource limits configuration APIs #67768

Conversation

RaduBerinde commented Jul 19, 2021

server: add TokenBucket connector API

sql: tenant resource limits configuration API

cockroach-teamcity commented Jul 19, 2021

RaduBerinde commented Jul 19, 2021

kernfeld-cockroach commented Jul 20, 2021

ajwerner left a comment

Choose a reason for hiding this comment

andy-kimball left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

andy-kimball left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

andy-kimball left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

andy-kimball commented Jul 21, 2021

RaduBerinde left a comment

Choose a reason for hiding this comment

RaduBerinde commented Jul 22, 2021

craig bot commented Jul 22, 2021

RaduBerinde commented Jul 22, 2021

craig bot commented Jul 22, 2021

RaduBerinde commented Jul 22, 2021

craig bot commented Jul 22, 2021

craig bot commented Jul 22, 2021